Support fine-grained SVS index - #1027
Draft
ethanglaser wants to merge 30 commits into
Draft
Conversation
* MOD-14916 Devirtualize distance + getElement on HNSW search hot path
MOD-14916 / LTK perf investigation.
Two virtual dispatches per HNSW candidate added between v2.10.21 and
v8.2.6 account for a measurable share of the KNN regression observed
in the LTK benchmarks (-38% throughput on Intel). Both are removed
here with the minimum possible change.
V1 - distance computation:
Every calcDistance() call goes through IndexCalculatorInterface's
vtable to reach DistanceCalculatorCommon, which then calls the
underlying SIMD function pointer. The intermediate vtable hop is
pure indirection; the concrete calculator class is fixed for the
life of an index.
Expose the underlying dist_func via a new pure-virtual
getDistFunc() on IndexCalculatorInterface, implemented by
DistanceCalculatorCommon. Cache the returned function pointer in
VecSimIndexAbstract at construction time and call it directly in
calcDistance(), bypassing the virtual dispatch.
V2 - vector fetch:
HNSWIndex::getDataByInternalId and BruteForceIndex::getDataByInternalId
call this->vectors->getElement(id), which is virtual through the
RawDataContainer base. DataBlocksContainer is the only concrete
implementation, and this->vectors is always a DataBlocksContainer
(created and owned by VecSimIndexAbstract's constructor).
Use a static_cast to DataBlocksContainer* plus a qualified call to
DataBlocksContainer::getElement to skip the vtable lookup.
No behavior change; per-candidate distance and neighbor-fetch calls
on HNSW / brute force search paths become direct function-pointer /
direct-member calls. Headers in index_factories, hnsw_serializer,
and brute_force_factory compile cleanly.
* MOD-14916 Inline DataBlocksContainer::getElement on HNSW search hot path
Follow-up to the previous V1/V2 devirt commit. The static_cast+qualified
call in getDataByInternalId removed the vtable lookup but left the larger
cost on the table: DataBlocksContainer::getElement was still defined in
data_blocks_container.cpp, so every per-candidate neighbor fetch still
paid a real out-of-line function call and a bounds-checked blocks.at()
lookup. Without LTO the compiler could neither inline the body nor hoist
the div/mod in the HNSW hot loop.
Move the definition into the header as inline and drop the .at() bounds
check to match the v2.10.21 baseline, which used unchecked operator[] and
was fully inlined into processCandidate.
Also add a getDistFunc() override to DistanceCalculatorDummy in
test_components.cpp so BUILD_TESTS still compiles after the pure virtual
added in the previous commit.
(cherry picked from commit 4ca500a)
Remove null characters from end of file
Upstream renamed the three thread-control methods on SVSIndexBase: getNumThreads -> getParallelism setNumThreads -> setParallelism getThreadPoolCapacity -> getPoolSize Adopt those names here ahead of merging upstream/main. This is a pure rename -- 32 lines across 6 files, mechanically verified by reversing the substitution and diffing against the parent commit. Method bodies deliberately keep this branch's threadpool API (size/resize/capacity), since VecSimSVSThreadPool here is still the per-index owned pool. Upstream reworked it into a process-wide singleton with thread renting, sized via VecSim_UpdateThreadPoolSize(); that is genuine divergence to reconcile in the merge, not something a rename should paper over. The point is to remove this conflict class before merging. The names collided on roughly half the affected lines without producing conflict markers, so git resolved some toward upstream and some toward here, yielding a tree that referenced methods it no longer declared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resolutions, by class: Thread pool. Upstream replaced the per-index owned VecSimSVSThreadPool with a process-wide singleton that rents threads, sized via VecSim_UpdateThreadPoolSize() and defaulting to parallelism 1. Took upstream throughout: the getParallelism/ setParallelism/getPoolSize bodies in svs.h (this branch's threadpool_.capacity() no longer exists), the constructor, and scheduleSVSIndexInit/GC, which now use createScheduledJobs() so the pool's reserve/release accounting stays balanced. Kept this branch's initSVSIndexWrapper as the callback -- Dmitry renamed updateSVSIndexWrapper, and the name upstream passes is no longer defined. Distance calculators. Upstream's DistanceDispatch supersedes this branch's getDistFunc()/cachedDistFunc (PR RedisAI#937): same vtable-avoidance goal, but generalized to stateful calculators, which the new SQ8 DistanceCalculatorWithNorm needs, plus asymmetric query distance. Took upstream wholesale for calculator.h, vec_sim_index.h, and test_components.cpp; this branch had touched those files only for the superseded caching, and no references to the old API remain. Concurrent index. Kept this branch throughout, since that is the point of it: SVSIndexBase::addVector (upstream dropped it; svs_tiered.h still needs it), ready(), atomic num_marked_deleted, the SegmentedBlocked/concurrent-namespace retargeting in svs_utils.h and svs_extensions.h, and the write-in-place delete-then-add path. That path keeps only updateJobMutex and not upstream's added mainIndexGuard -- the concurrent backend serializes writes against readers itself. Upstream's executeInsertJob-adjacent conflict was a mis-alignment: the text it offered belongs to the batch-drain function that initSVSIndex() replaces. Carried upstream changes that would otherwise have been lost to that restructuring: the GCJob::before_run_gc tracing hook (fired before taking updateJobMutex, so a test callback cannot deadlock against it) and the empty-batch guard around the backend write in initSVSIndex(), where setParallelism(0) is not a valid request against the shared pool. Tests. Took upstream's expectations: SVSParams.num_threads is now deprecated and ignored with a warning, so deriving expected capacity from it is no longer valid. deps/ScalableVectorSearch. Restored as a proper submodule gitlink at upstream's 7786d43b, discarding commit 8265a7a's symlink into a developer's home directory, which was dangling for everyone else. Not verified by a build: this host has GCC 11.4 and no container runtime, and SVS needs GCC 13+. Audited statically instead -- no conflict markers, no orphaned references to removed APIs, and every SVSIndexBase method used in svs_tiered.h is declared in svs.h. That last check is the one the first attempt at this merge failed: half the thread-API collisions produced no conflict markers, so git resolved some lines toward each side and left the tree calling methods it no longer declared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Dmitry Razdoburdin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
added 2 commits
September 7, 2026 01:51
…/VectorSimilarity into dev/eglaser-lockfree
ethanglaser
commented
Sep 8, 2026
| // initSVSIndexWrapper() is called. | ||
| { | ||
| std::lock_guard lock(this->flatIndexGuard); | ||
| if (this->frontendIndex->isLabelExists(label)) { |
Collaborator
Author
There was a problem hiding this comment.
This condition should have the same guard as line 1002, otherwise every vector after the first of a multi-value doc wipes siblings out of the frontend buffer
Suggested change
| if (this->frontendIndex->isLabelExists(label)) { | |
| if (!this->frontendIndex->isMultiValue() && this->frontendIndex->isLabelExists(label)) { |
This update should lead to the redisearch TestIndexMultiValueJsonReload tests go green
| } | ||
| // reset journal to the current frontend index state | ||
| swaps_journal.clear(); | ||
| deleted_labels_journal.clear(); |
Collaborator
Author
There was a problem hiding this comment.
deleted_labels_journal, swaps_journal, applySwapsToLabelsArray usage has been removed in current state - if this functionality is replaced and not going to be re-integrated into the code, then delete instantiation/definition of these
But it may be necessary to restore usage here to address review comments above
Comment on lines
+790
to
+793
| memcpy(blob_copy.get(), this->frontendIndex->getDataByInternalId(job->id), data_size); | ||
| this->flatIndexGuard.unlock_shared(); | ||
|
|
||
| svs_index->addVector(blob_copy.get(), job->label); |
Collaborator
Author
There was a problem hiding this comment.
Once a job passes the job->isValid check at line 776 and releases the guard at 791, nothing orders its backend insert against a concurrent deleteVector. deleteVector (line 1133) invalidates pending insert jobs under flatIndexGuard, but that's too late for a job already past 776: the delete runs to completion, its backendIndex->deleteVector(label) returns 0 because the vector isn't in the backend yet, and then line 793 puts it there. Result is a backend entry for a deleted doc that is not marked deleted, so GC never reclaims it and label accounting can't see it.
This was previously addressed in 2 ways, both of which are removed in this PR: mainIndexGuard held exclusively across the backend add and the backend delete (significant usage reduction in this PR), and the deletions journal (usage removed in this PR)
This resolution should lead to the redisearch test_delete_during_background_indexing tests go green
Comment on lines
+1172
to
+1177
| if (deleted > 0) { | ||
| if (this->getWriteMode() == VecSim_WriteInPlace) { | ||
| GetSVSIndex()->consolidate({label}); | ||
| } else { | ||
| scheduleSVSIndexConsolidate(label); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Redisearch test_gc tests are failing due to increased memory from per-deleted label consolidation, as well as marked-delete count and pending-job count assertions - is it possible to do this on batches or have a single deferred job that accumulates labels?
Collaborator
There was a problem hiding this comment.
it will return latency spikes back.
I think the test should be changed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds support for using newly-added separate fine-grained SVS index with minor corresponding test revisions
Includes SVS 0.5.0 rc1 binaries, which will eventually be swapped out for the official release binaries after validation